Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds several new FinishReason cases, primarily related to image generation and tool usage, and includes corresponding integration tests. The changes are generally good, but I've identified a minor grammatical error in a documentation comment and a more significant issue in one of the new integration tests. The test for Imagen safety filtering is incorrectly structured and will fail as it doesn't account for an expected error being thrown. I've provided a corrected implementation for the test.
Generated by 🚫 Danger |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds several new FinishReason cases to the SDK, along with corresponding unit and integration tests. The changes to the FinishReason enum and the new integration test for Gemini models are well-implemented. However, I've identified a logical issue in the new Imagen integration test and an opportunity to improve maintainability in the new unit test for decoding finish reasons. My detailed feedback is in the comments below.
| let response = try await model.generateImages(prompt: imagePrompt) | ||
|
|
||
| #expect(response.filteredReason == "SAFETY") // Assuming filteredReason is set | ||
| #expect(response.images.isEmpty) // No images should be generated |
There was a problem hiding this comment.
This test appears to have a logical contradiction. It expects the call to model.generateImages(prompt: imagePrompt) to succeed, but then asserts that response.images.isEmpty.
Based on the implementation of ImagenGenerationResponse.init(from:), an ImagenImagesBlockedError is thrown if the images array is empty. Therefore, this test should expect an error to be thrown rather than a successful response.
Please consider refactoring this test to use await #expect { ... } throws: { ... } to correctly handle and verify the expected error, similar to the generateImage_allImagesFilteredOut test.
await #expect {
_ = try await model.generateImages(prompt: imagePrompt)
} throws: { error in
let imagenError = try #require(error as? ImagenImagesBlockedError)
#expect(imagenError.message == "SAFETY")
}| func testFinishReason_decoding() throws { | ||
| let decoder = JSONDecoder() | ||
|
|
||
| // Test LANGUAGE | ||
| var json = createResponseJSON(finishReason: "LANGUAGE") | ||
| var response = try decoder.decode(GenerateContentResponse.self, from: json) | ||
| XCTAssertEqual(response.candidates.first?.finishReason, .language) | ||
|
|
||
| // Test UNEXPECTED_TOOL_CALL | ||
| json = createResponseJSON(finishReason: "UNEXPECTED_TOOL_CALL") | ||
| response = try decoder.decode(GenerateContentResponse.self, from: json) | ||
| XCTAssertEqual(response.candidates.first?.finishReason, .unexpectedToolCall) | ||
|
|
||
| // Test TOO_MANY_TOOL_CALLS | ||
| json = createResponseJSON(finishReason: "TOO_MANY_TOOL_CALLS") | ||
| response = try decoder.decode(GenerateContentResponse.self, from: json) | ||
| XCTAssertEqual(response.candidates.first?.finishReason, .tooManyToolCalls) | ||
|
|
||
| // Test MISSING_THOUGHT_SIGNATURE | ||
| json = createResponseJSON(finishReason: "MISSING_THOUGHT_SIGNATURE") | ||
| response = try decoder.decode(GenerateContentResponse.self, from: json) | ||
| XCTAssertEqual(response.candidates.first?.finishReason, .missingThoughtSignature) | ||
|
|
||
| // Test MALFORMED_RESPONSE | ||
| json = createResponseJSON(finishReason: "MALFORMED_RESPONSE") | ||
| response = try decoder.decode(GenerateContentResponse.self, from: json) | ||
| XCTAssertEqual(response.candidates.first?.finishReason, .malformedResponse) | ||
|
|
||
| // Test IMAGE_SAFETY | ||
| json = createResponseJSON(finishReason: "IMAGE_SAFETY") | ||
| response = try decoder.decode(GenerateContentResponse.self, from: json) | ||
| XCTAssertEqual(response.candidates.first?.finishReason, .imageSafety) | ||
|
|
||
| // Test IMAGE_PROHIBITED_CONTENT | ||
| json = createResponseJSON(finishReason: "IMAGE_PROHIBITED_CONTENT") | ||
| response = try decoder.decode(GenerateContentResponse.self, from: json) | ||
| XCTAssertEqual(response.candidates.first?.finishReason, .imageProhibitedContent) | ||
|
|
||
| // Test IMAGE_OTHER | ||
| json = createResponseJSON(finishReason: "IMAGE_OTHER") | ||
| response = try decoder.decode(GenerateContentResponse.self, from: json) | ||
| XCTAssertEqual(response.candidates.first?.finishReason, .imageOther) | ||
|
|
||
| // Test NO_IMAGE | ||
| json = createResponseJSON(finishReason: "NO_IMAGE") | ||
| response = try decoder.decode(GenerateContentResponse.self, from: json) | ||
| XCTAssertEqual(response.candidates.first?.finishReason, .noImage) | ||
|
|
||
| // Test IMAGE_RECITATION | ||
| json = createResponseJSON(finishReason: "IMAGE_RECITATION") | ||
| response = try decoder.decode(GenerateContentResponse.self, from: json) | ||
| XCTAssertEqual(response.candidates.first?.finishReason, .imageRecitation) | ||
| } |
There was a problem hiding this comment.
The test testFinishReason_decoding contains a lot of repetitive code for each FinishReason case, which makes it verbose and harder to maintain.
To improve readability and maintainability, I suggest refactoring this test to be data-driven. You can create an array of test cases (tuples containing the reason string and the expected FinishReason enum case) and iterate through them in a single loop.
func testFinishReason_decoding() throws {
let decoder = JSONDecoder()
let testCases: [(String, FinishReason)] = [
("LANGUAGE", .language),
("UNEXPECTED_TOOL_CALL", .unexpectedToolCall),
("TOO_MANY_TOOL_CALLS", .tooManyToolCalls),
("MISSING_THOUGHT_SIGNATURE", .missingThoughtSignature),
("MALFORMED_RESPONSE", .malformedResponse),
("IMAGE_SAFETY", .imageSafety),
("IMAGE_PROHIBITED_CONTENT", .imageProhibitedContent),
("IMAGE_OTHER", .imageOther),
("NO_IMAGE", .noImage),
("IMAGE_RECITATION", .imageRecitation),
]
for (reasonString, expectedReason) in testCases {
let json = createResponseJSON(finishReason: reasonString)
let response = try decoder.decode(GenerateContentResponse.self, from: json)
XCTAssertEqual(
response.candidates.first?.finishReason,
expectedReason,
"Failed to decode finishReason '\(reasonString)'"
)
}
}1 participant
No description provided.